feat: Adding support for shared-requestor #104

heyvister1 · 2025-06-13T13:40:51Z

The motivation of the following code change is to utilize same node-maintenance object(s), to efficiently synchronize required node operations, by multiple requestors. It will help to reduce consecutive node operation cycles to a single operation cycle.

First requestor which creates node-maintenance obj is its owner, and will be in-charge of creating/deleting node-maintenance obj.
Second requestor will check for existing node-maintenance obj, if so, it will add itself under spec.additionalRequestors list.

copy-pr-bot · 2025-06-13T13:40:55Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

adrianchiris · 2025-06-16T10:25:41Z

docs/automatic-ofed-upgrade.md

@@ -108,6 +108,21 @@ controller manager watchers:
 			builder.WithPredicates(requestor.NewConditionChangedPredicate(setupLog,
 		requestorOpts.MaintenanceOPRequestorID))).
 ```
+
+The requestor mode supports a `joint-requestor` flow where multiple operators can coordinate node maintenance operations:


nit: any chance to modify the terminology to: shared-requestor or shared-node-maintenenace

consider putting this in a sub-section of requestor

adrianchiris · 2025-06-16T12:18:54Z

docs/automatic-ofed-upgrade.md

+Assumptions:
+1. Cluster admin, which requires requestor joint mode, needs to make sure that all operators, utilizing maintenance OP, use same upgrade policy specs (same drainSpec).
+2. To be able to accommodate both GPU/Network drivers upgrade, `DrainSpec.PodSelector` should be set accordingly (hard-coded). 
+ * podSelector: `nvidia.com/ofed-driver-upgrade-drain.skip!=true,nvidia.com/gpu-driver-upgrade-drain.skip!=true`


just making sure, drain pkg will treat this as an OR right ?

It will be treated as AND condition.
Check the following example (I have tested it)

1. pod "foo" - nvidia.com/ofed-driver-upgrade-drain.skip=true label 2. pod "bar" - nvidia.com/gpu-driver-upgrade-drain.skip=true label 3. pod "baz" - no label

Pod "foo" (has nvidia.com/ofed-driver-upgrade-drain.skip=true):
nvidia.com/ofed-driver-upgrade-drain.skip!=true → FALSE (because it equals true)
nvidia.com/gpu-driver-upgrade-drain.skip!=true → TRUE (because this label doesn't exist, so it's not equal to true)
Result: FALSE AND TRUE = FALSE

Pod "bar" (has nvidia.com/gpu-driver-upgrade-drain.skip=true):
nvidia.com/ofed-driver-upgrade-drain.skip!=true → TRUE (because this label doesn't exist, so it's not equal to true)
nvidia.com/gpu-driver-upgrade-drain.skip!=true → FALSE (because it equals true)
Result: TRUE AND FALSE = FALSE

Pod "baz" (no labels):
nvidia.com/ofed-driver-upgrade-drain.skip!=true → TRUE (because this label doesn't exist, so it's not equal to true)
nvidia.com/gpu-driver-upgrade-drain.skip!=true → TRUE (because this label doesn't exist, so it's not equal to true)
Result: TRUE AND TRUE = TRUE

ack, thx for the clarification so its equivalent to:

nvidia.com/ofed-driver-upgrade-drain.skip=true OR nvidia.com/gpu-driver-upgrade-drain.skip=true

adrianchiris · 2025-06-16T13:38:20Z

pkg/upgrade/upgrade_requestor.go

+		if _, exists := nm.Labels[GetUpgradeRequestorLabelKey(m.opts.MaintenanceOPRequestorID)]; !exists {
+			nm.Labels[GetUpgradeRequestorLabelKey(m.opts.MaintenanceOPRequestorID)] = trueString
+		}
+		err := m.K8sClient.Patch(ctx, nm, client.MergeFrom(originalNm))


you say in comments that optimistic lock should be used (rightfully so), but i dont see you using:

client.MergeFromWithOptions(originalNm, client.MergeFromWithOptimisticLock{})

in patch calls here an below

This was missing in my latest commit

adrianchiris · 2025-06-16T13:40:35Z

pkg/upgrade/upgrade_requestor.go

@@ -306,6 +324,114 @@ func (m *RequestorNodeStateManagerImpl) ProcessUpgradeRequiredNodes(

 	return nil
 }
+func (m *RequestorNodeStateManagerImpl) createOrUpdateNodeMaintenance(ctx context.Context,
+	nodeState *NodeUpgradeState) error {
+	// 1. check if nodeMaintenance obj exists.


are these comments relevant in createOrUpdateNodeMaintenance ?

maybe need to move some of it to docs ?

adrianchiris · 2025-06-16T13:42:33Z

docs/automatic-ofed-upgrade.md

@@ -108,6 +108,21 @@ controller manager watchers:
 			builder.WithPredicates(requestor.NewConditionChangedPredicate(setupLog,
 		requestorOpts.MaintenanceOPRequestorID))).
 ```
+
+The requestor mode supports a `joint-requestor` flow where multiple operators can coordinate node maintenance operations:


consider putting this in a sub-section of requestor

adrianchiris · 2025-06-16T13:47:26Z

pkg/upgrade/upgrade_requestor.go

@@ -438,12 +557,12 @@ func GetRequestorOptsFromEnvs() RequestorOptions {
 	if os.Getenv("MAINTENANCE_OPERATOR_REQUESTOR_ID") != "" {
 		opts.MaintenanceOPRequestorID = os.Getenv("MAINTENANCE_OPERATOR_REQUESTOR_ID")
 	} else {
-		opts.MaintenanceOPRequestorID = "nvidia.operator.com"
+		opts.MaintenanceOPRequestorID = DefaultNodeMaintenanceRequestorID


its not really a great ux IMO to enable this feature when the two env vars are unset.

I'll be checking if DefaultNodeMaintenanceRequestorID is used before using share-requestor flow

heyvister1 · 2025-06-19T10:03:45Z

@cdesiniotis, @tariq1890 please help to review this PR

Signed-off-by: Ido Heyvi <[email protected]>

adrianchiris · 2025-06-25T13:27:56Z

docs/automatic-ofed-upgrade.md

+1. Each operator adds its dedicated operator label to the nodeMaintenance object
+2. When a nodeMaintenance object exists, additional operators append their requestorID to the spec.AdditionalRequestors list
+3. During `uncordon-required` completion:
+   - Non-owning operators remove themselves from spec.AdditionalRequestors list using optimistic locking


who is the "owning operator" ?

it is the operator that managed to create the NodeMaintenance object.
which means for a given NodeMaintenance obj (name of obj is the same in the shared-requestor mode for all cooperating operators) its the operator whose RequestorID is set in spec.requestorID

who is a "non owning" operator ?

its the operator that did not create the NodeMaintenances object, which means for a given NodeMaintenance obj its the operator whose RequestorID is present in spec.AdditionalRequestors

i would define these two roles and how its determined according to the content of NodeMaintenance obj.

adrianchiris · 2025-06-25T14:36:24Z

pkg/upgrade/upgrade_inplace.go

@@ -130,6 +130,10 @@ func (m *InplaceNodeStateManagerImpl) ProcessUncordonRequiredNodes(
 		if nodeState.NodeMaintenance != nil {
 			continue
 		}
+		// check if if node upgrade is handled by requestor mode, if so node uncordon will be performed by requestor flow


should we remove nodeState.NodeMaintenance != nil check?

adrianchiris · 2025-06-25T14:49:27Z

pkg/upgrade/upgrade_requestor.go

@@ -120,8 +124,18 @@ func (p ConditionChangedPredicate) Update(e event.TypedUpdateEvent[client.Object
 		return false
 	}

-	// check for matching requestor ID
-	if newO.Spec.RequestorID != p.requestorID {
+	// check if requestor label exists, if not ignore event


can we skip this label and just check if the requestorID is in spec.requestorID or in spec.additionalRequestors list ?
one less label we need to worry about.

also would be great to have a separate predicate for checking if the nodeMaintenance is relevant for the requestor.

you can use controller-runtime/pkg/predicate:NewPredicateFuncs() instead of defining a new struct + implementing all callbacks.

adrianchiris · 2025-06-25T15:14:51Z

pkg/upgrade/upgrade_requestor.go

@@ -48,6 +48,10 @@ const (
 	MaintenanceOPEvictionGPU = "nvidia.com/gpu-*"
 	// MaintenanceOPEvictionRDMA is a default filter for Network OP pods eviction
 	MaintenanceOPEvictionRDMA = "nvidia.com/rdma*"
+	// DefaultNodeMaintenanceRequestorID is a default requestor ID for NVIDIA operators, in nodeMaintenance object
+	DefaultNodeMaintenanceRequestorID = "nvidia.operator.com"


consider removing the default requestor ID, i think every consumer of this lib (an operator) should provide its own ID. less risk of defaulting to the same ID.

adrianchiris · 2025-06-26T06:17:32Z

pkg/upgrade/upgrade_requestor.go

@@ -165,13 +179,16 @@ func SetDefaultNodeMaintenance(opts RequestorOptions,
 func (m *RequestorNodeStateManagerImpl) NewNodeMaintenance(nodeName string) *maintenancev1alpha1.NodeMaintenance {
 	nm := defaultNodeMaintenance.DeepCopy()
 	nm.Name = m.getNodeMaintenanceName(nodeName)
+	// mark nodeMaintenance object as updated by the requestor


as discussed, i think we can drop this label ?

adrianchiris · 2025-06-26T06:26:33Z

pkg/upgrade/upgrade_requestor.go

+			return nil
+		}
+
+		m.Log.V(consts.LogLevelInfo).Info("appending new requestor", nm.Spec.RequestorID, "under 'AdditionalRequestors' list")


nit: restructure this and use m.opts.MaintenanceOPRequestorID

m.Log.V(consts.LogLevelInfo).Info("appending new requestor under AdditionalRequestors", "requestor", m.opts.MaintenanceOPRequestorID, "nodeMaintenance", client.ObjectKeyFromObject(nm))

adrianchiris · 2025-06-26T06:27:11Z

pkg/upgrade/upgrade_requestor.go

+		if nm.Labels == nil {
+			nm.Labels = make(map[string]string)
+		}
+		// only set label if it doesn't exist


nit: we can drop label ?

adrianchiris · 2025-06-26T06:30:36Z

pkg/upgrade/upgrade_requestor.go

+		// check if object is owned by deleting requestor, if so proceed to deletion
+		if nm.Spec.RequestorID == m.opts.MaintenanceOPRequestorID {
+			m.Log.V(consts.LogLevelInfo).Info("deleting node maintenance",
+				nodeState.NodeMaintenance.GetName(), nodeState.NodeMaintenance.GetNamespace())


nit: m.Log.V().Info("....", "nodeMaintenance", client.ObjectKeyFromObject(nodeState.NodeMaintenance))

adrianchiris · 2025-06-26T06:31:53Z

pkg/upgrade/upgrade_requestor.go

+				nodeState.NodeMaintenance.GetName(), nodeState.NodeMaintenance.GetNamespace())
+			err := m.deleteNodeMaintenance(ctx, nodeState)
+			if err != nil {
+				m.Log.V(consts.LogLevelWarning).Error(


nit: "failed to delete NodeMaintenance, node uncordon failed", "nodeMaintenance", client.ObjectKeyFromObject(nodeState.NodeMaintenance)

adrianchiris · 2025-06-26T06:33:21Z

pkg/upgrade/upgrade_requestor.go

+				})
+				// remove requestorID label from the object
+				// only remove label if it exists
+				if nm.Labels != nil {


nit: drop label ?

adrianchiris · 2025-06-26T06:34:34Z

pkg/upgrade/upgrade_requestor.go

+				patch := client.MergeFromWithOptions(originalNm, client.MergeFromWithOptimisticLock{})
+				err := m.K8sClient.Patch(ctx, nm, patch)
+				if err != nil {
+					return fmt.Errorf("failed to update nodeMaintenance. %v", err)


nit: lets be a bit more verbose: "failed to remove requestor from additionalRequestors. failed to patch nodeMaintenance %s. %w", client.ObjectKeyFromObject(nodeState.NodeMaintenenace), err

adrianchiris · 2025-06-26T06:58:40Z

pkg/upgrade/upgrade_requestor.go

-		m.Log.V(consts.LogLevelDebug).Info("deleting node maintenance",
-			nodeState.NodeMaintenance.GetName(), nodeState.NodeMaintenance.GetNamespace())
-		// skip in case node undergoes uncordon by inplace flow
+		// skip in case node undergoes uncordon by in-place flow


i think the flow here should be:

func (m *RequestorNodeStateManagerImpl) ProcessUncordonRequiredNodes( ctx context.Context, currentClusterState *ClusterUpgradeState) error { m.Log.V(consts.LogLevelInfo).Info("ProcessUncordonRequiredNodes") for _, nodeState := range currentClusterState.NodeStates[UpgradeStateUncordonRequired] { // check if if node upgrade is handled by requestor mode, if not, its handled by in-place flow so nothing to do if _, exists := nodeState.Node.Annotations[GetUpgradeRequestorModeAnnotationKey()]; !exists { continue } if nodeState.NodeMaintenance == nil { // only when nodeMaintenance obj is deleted, node state should be updated to 'upgrade-done' err := m.NodeUpgradeStateProvider.ChangeNodeUpgradeState(ctx, nodeState.Node, UpgradeStateDone) if err != nil { m.Log.V(consts.LogLevelError).Error( err, "Failed to change node upgrade state", "state", UpgradeStateDone) return err } // remove requestor upgrade annotation err = m.NodeUpgradeStateProvider.ChangeNodeUpgradeAnnotation(ctx, nodeState.Node, GetUpgradeRequestorModeAnnotationKey(), "null") if err != nil { return fmt.Errorf("failed to remove '%s' annotation from node %s. %w", GetUpgradeRequestorModeAnnotationKey(), nodeState.Node.Name, err) } return nil } err := m.deleteOrUpdateNodeMaintenance(ctx, nodeState) if err != nil { m.Log.V(consts.LogLevelWarning).Error( err, "failed to delete or update NodeMaintenance. Node uncordon failed", "nodeMaintenance", client.ObjectKeyFromObject(nodeState.NodeMaintenance), "node", nodeState.Node.Name) return err } } return nil }

in case of the node's requestor mode annotation does not exist, its fully handled by in place (i.e uncordon node and move to upgrade state done)

heyvister1 force-pushed the joint-requestor branch 4 times, most recently from 4b4c42c to e6c5941 Compare June 15, 2025 13:01

adrianchiris reviewed Jun 16, 2025

View reviewed changes

heyvister1 force-pushed the joint-requestor branch from e6c5941 to ee07667 Compare June 19, 2025 05:58

heyvister1 requested a review from adrianchiris June 19, 2025 05:59

heyvister1 changed the title ~~feat: Adding support for joint requestors~~ feat: Adding support for shared-requestor Jun 19, 2025

heyvister1 force-pushed the joint-requestor branch 4 times, most recently from b062823 to 3eaf949 Compare June 19, 2025 14:22

feat: Adding support for shared-requestor flow

246770e

Signed-off-by: Ido Heyvi <[email protected]>

heyvister1 force-pushed the joint-requestor branch from 3eaf949 to 246770e Compare June 19, 2025 14:23

adrianchiris reviewed Jun 25, 2025

View reviewed changes

adrianchiris reviewed Jun 26, 2025

View reviewed changes

feat: Adding support for shared-requestor #104

Are you sure you want to change the base?

feat: Adding support for shared-requestor #104

Uh oh!

Conversation

heyvister1 commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

copy-pr-bot bot commented Jun 13, 2025

Uh oh!

adrianchiris Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianchiris Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heyvister1 commented Jun 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

heyvister1 commented Jun 13, 2025 •

edited

Loading

adrianchiris Jun 16, 2025 •

edited

Loading

adrianchiris Jun 16, 2025 •

edited

Loading